-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd-buildextend-live: update kargs_json
when moving EFI grub.cfg
#3169
Conversation
It's more legible that way.
This allows the glob to be relative to the live ISO root. Prep for future patch which relies on this.
Otherwise, code lower down will remove the that `grub.cfg` from the list of kargs files. This in turn will prevent `coreos-installer iso kargs` from affecting kargs on UEFI boots. Fixes 3991e6f ("cmd-buildextend-live: always change dir `EFI/{vendor}/` to correct vendor id").
Rather than silently deleting files that no longer exist, flip this around so that we assert they all exist. This will then ensure that new code moving `grub.cfg` will also have to update `kargs_json`.
Let's do coreos/coreos-ci-lib#134 first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
On FCOS, the EFI directory is already correctly named in the src config repo. So there's no need to do any renames at all. The previous code still worked because `renameat(2)` with the src and dest being the same file is a no-op. This is a bit too implicit though. Let's just add a check to be more clear. This avoids us also unnecessarily iterating over `kargs_json['files']`. While here, add a print statement when this logic kicks in.
The reason RHCOS hit this but not FCOS is that in RHCOS we did openshift/os@ Still, I added another commit to make all this more explicit. |
if len(grubfilepath) != 1: | ||
raise Exception(f'Found != 1 grub.cfg files: {grubfilepath}') | ||
srcpath = os.path.dirname(grubfilepath[0]) | ||
os.renames(srcpath, os.path.join(os.path.dirname(srcpath), vendor_ids[0])) | ||
if srcpath != f'EFI/{vendor_id}': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this could use a comment.
This isn't for this PR (this PR is just fixing an existing problem), but... this whole "rename the directory to a conforming path" feels really weird. It feels like a change that needs to be made somewhere else (C9s?). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Otherwise, code lower down will remove the that
grub.cfg
from the listof kargs files. This in turn will prevent
coreos-installer iso kargs
from affecting kargs on UEFI boots.
Fixes 3991e6f ("cmd-buildextend-live: always change dir
EFI/{vendor}/
to correct vendor id").